Add soft logout possibility for OIDC backchannel_logout #15976
Conversation
hachem2001
changed the title
hachem2001
changed the title
|
For cross-linking, there was a thread on the original PR about this: #11414 (comment) |
| * `backchannel_logout_is_soft`: by default all OIDC Back-Channel Logouts correspond to hard logouts on | ||
| the server side. This may not leave users the ability to recover their encryption keys before being logged-out. | ||
| This can be set to `true` to treat all OIDC Back-Channel logouts as soft-logouts, | ||
| allowing users to reconnect to the same device if necessary to recover their keys. | ||
| Defaults to `false`. |
There was a problem hiding this comment.
@sandhose Do you have any thoughts on if it makes sense for this to be configurable or not? (Previous conversation at #11414 (comment))
The backchannel-logout is used when a user manually logs out of an IdP? Or is it also used when a user's session expires with an IdP?
The former seems like it would make sense with the current (hard-)logout, while the latter should likely be a soft-logout.
There was a problem hiding this comment.
I think keeping the option to do both is good. Usually, a backchannel logout happens because the user expressed the intent of logging out, but it might also happen when for example an admin kicks out a user to force logging back in, and in this case you might want to do a soft_logout in clients.
There was a problem hiding this comment.
It's indeed for the case where the user doesn't launch the backchannel logout on the server directly. Backchannel logout on synapse can occur without the user intentionally provoking it in some cases.
I made this as an option for safety upon upgrade, not to break any OIDC behavior servers currently rely on. Although I don't envision cases where a server would prefer a hard-logout for users, it's better to have the choice I suppose.
Another alternative would be to set backchannel_logout_is_soft to true by default, and in a further version eventually rule out the option and make it default behavior (basically an ephemeral temporary option). This is the best compromise in my opinion.
| * `backchannel_logout_is_soft`: by default all OIDC Back-Channel Logouts correspond to hard logouts on | ||
| the server side. This may not leave users the ability to recover their encryption keys before being logged-out. | ||
| This can be set to `true` to treat all OIDC Back-Channel logouts as soft-logouts, | ||
| allowing users to reconnect to the same device if necessary to recover their keys. | ||
| Defaults to `false`. |
There was a problem hiding this comment.
I think keeping the option to do both is good. Usually, a backchannel logout happens because the user expressed the intent of logging out, but it might also happen when for example an admin kicks out a user to force logging back in, and in this case you might want to do a soft_logout in clients.
| @@ -1232,6 +1232,91 @@ async def revoke_sessions_for_provider_session_id( | |||
| ) | |||
| await self._device_handler.delete_devices(user_id, [device_id]) | |||
|
|
|||
| async def invalidate_sessions_for_provider_session_id( | |||
There was a problem hiding this comment.
This feels like it should be instead a parameter on the revoke_sessions_for_provider_session_id method instead, because most of the logic here is duplicated
There was a problem hiding this comment.
I agree, though I avoided that in case this redundancy improves visibility, and in case we don't want to change the behavior of one of the functions in the long-run.
But merging it with revoke_sessions_for_provider_session_id by adding a new parameter (and modifying all subsequent calls of the function) is possible.
There was a problem hiding this comment.
I suspect it would improve readability if there's less duplicated code, so we can see what the differences are based on the flag.
There was a problem hiding this comment.
@hachem2001 Do you have interest in making this change?
|
The github-ci tests don't seem to fail for me, I'm not sure how to fix that on here. |
I don't think it has to do with being up-to-date -- there's been a few flakey tests in our CI recently. It is usually easier to re-run the single job instead of merging in develop (which then runs all the tests again). |
guimard
force-pushed
the
oidc-soft-logout
branch
from
a2ae60b
to
671fc2d
Compare
Pull Request Checklist
ISSUE : When doing a backchannel logout using an OIDC provider, a user may lose any non backed-up keys.
PROPOSED FEATURE : Allow to set the backchannel logout induced by an OIDC to be a soft-logout, thus allowing the user to reconnect to the same device and recover his session if necessary.
IMPLEMENTATION : Allow setting OIDC backchannel logout to be a soft-logout in OIDC provider configuration via
backchannel_logout_is_softwhich defaults to false.Signed-off-by: OUERTANI Mohamed Hachem [email protected]